-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Provide EVM support for some linera features #3847
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Provide EVM support for some linera features #3847
Conversation
linera-execution/src/evm/revm.rs
Outdated
} | ||
|
||
impl PrecompileTag { | ||
fn from_u8(tag: u8) -> Result<Self, String> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way we could avoid this boilerplate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(For instance using bincode
or bcs
serialization)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, there are ways, but they are not necessarily as simple as 10 lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. It's also a question of coding style: We consistently try to avoid boilerplate in the rest of the codebase
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, changed.
8220e8f
to
4b79b80
Compare
4b79b80
to
c258fc0
Compare
// (0,0): chain_id | ||
// (0,1): application_creator_chain_id | ||
// (0,2): chain_ownership | ||
// (0,3): read data blob | ||
// (0,4): assert data blob exists | ||
// (1,0): try_call_application | ||
// (1,1): validation round | ||
// (1,2): send_message | ||
// (1,3): message_id | ||
// (1,4): message_is_bouncing | ||
// (2,0): try_query_application |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reasoning for choosing this particular encoding? each element of the tuple is uint8
so holds numbers up to 255. Wouldn't it be easier to simply keep the indexes monotonically increase?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It has to match the bcs encoding of the type PrecompileTag
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it also means that if we change the order of enum (variants) this would also have to change?
Why not manually assign values to it?
#[repr(u8)]
enum PrecompileTag {
ChainId = 0x01,
ApplicationCreatorChainId = 0x02
...
}
etc? It is automatically documented that we assign these indices for a reason - the diff is clearer as well.
strum
or num_enum
crates provide the macros to derive from
/into
impls.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This organization of the PrecompileTag
is the one before this PR.
The problem is that some runtime operations are available in both Contract
and Service
, some are available only in Contract
and some only in Service
.
With the proposed reorganization, things become much clearer and the code is better organized:
#[derive(Debug, Serialize, Deserialize)]
enum PrecompileTag {
Base(BasePrecompileTag),
Contract(ContractPrecompileTag),
Service(ServicePrecompileTag),
}
But since we do not want to have boilerplate code, this implies having 2 bytes instead of just 1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. I still think it'd be good to have some better safety precautions here.
Maybe for the time being add a longer comment here that these indices have to match the serde encoding of Precompile tag. Also, maybe a comment on PrecompileTag
with a warning that things should not be reorganised b/c they can break the integration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving but please update the comment on the indices for the Linera FFI.
Motivation
The Linera features need to be provided for EVM. Some of this is done here.
Proposal
The following is provided:
The precompile is extended for that purpose and the handling of the various cases is reorganized for clarity.
Test Plan
A test for the features has been provided in the CI.
Release Plan
The goal is to provide all this for TestNet 3.
Links
None.